Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: decision record about configuration injection #4611

Conversation

paullatzelsperger
Copy link
Member

What this PR changes/adds

Adds a decision record that outlines the configuration injection feature

Linked Issue(s)

Closes # <-- insert Issue number if one exists

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we have errors reported, can we not use exceptions and stack traces? I would prefer a severe message containing the class requiring the injection point, field name, and expected type. That will be easier than reading stacktraces.

@paullatzelsperger
Copy link
Member Author

paullatzelsperger commented Nov 7, 2024

When we have errors reported, can we not use exceptions and stack traces? I would prefer a severe message containing the class requiring the injection point, field name, and expected type. That will be easier than reading stacktraces.

The runtime won't boot in the case a dependency or a mandatory config setting is not satisfied, so an exception is perfectly fine, and the information you mentioned is indeed logged. In addition, changing the way dependency errors are reported would impact areas of code outside of the scope of this DR, so that would be a separate discussion anyway. My goal here was to align the error reporting of config injection with that of dependency injection.

@jimmarino
Copy link
Contributor

When we have errors reported, can we not use exceptions and stack traces? I would prefer a severe message containing the class requiring the injection point, field name, and expected type. That will be easier than reading stacktraces.

The runtime won't boot in the case a dependency or a mandatory config setting is not satisfied, so an exception is perfectly fine, and the information you mentioned is indeed logged. In addition, changing the way dependency errors are reported would impact areas of code outside of the scope of this DR, so that would be a separate discussion anyway. My goal here was to align the error reporting of config injection with that of dependency injection.

That's not what I'm saying. When the missing config error is encountered, it should be reported to a context where it is collated, the loading for that extension is aborted, and loading then continues. After the phase is complete, the context should be checked, and the recorded errors (not exception) reported to the monitor. After the errors are reported, an exception can be thrown (or the runtime terminated).

@ndr-brt
Copy link
Member

ndr-brt commented Nov 7, 2024

That's not what I'm saying. When the missing config error is encountered, it should be reported to a context where it is collated, the loading for that extension is aborted, and loading then continues. After the phase is complete, the context should be checked, and the recorded errors (not exception) reported to the monitor. After the errors are reported, an exception can be thrown (or the runtime terminated).

how could loading continues if an extension is aborted? other extensions could be dependent of that one so likely other kind of errors could appear.
As it is done for @Injection points, it should fail at the first inconsistency

@jimmarino
Copy link
Contributor

That's not what I'm saying. When the missing config error is encountered, it should be reported to a context where it is collated, the loading for that extension is aborted, and loading then continues. After the phase is complete, the context should be checked, and the recorded errors (not exception) reported to the monitor. After the errors are reported, an exception can be thrown (or the runtime terminated).

how could loading continues if an extension is aborted? other extensions could be dependent of that one so likely other kind of errors could appear. As it is done for @Injection points, it should fail at the first inconsistency

The point of this is to collate all missing configuration points. This may need to perform config injection before service injection to achieve that. During config injection, if there are errors, all operations on the extension in question should cease and the booter moves to the next extension.

@paullatzelsperger paullatzelsperger merged commit c6bb542 into eclipse-edc:main Nov 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants